Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix setting Lwt_process env on Windows #967

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mroch
Copy link

@mroch mroch commented Oct 19, 2022

env is an "environment block": a null-terminated block of null-terminated strings. using caml_stat_strdup_to_os on it doesn't work, it only copies the first string.

instead, we need to use an explicit length (caml_string_length(env)), but there's no public *_to_os function for this so we have to use the internal caml_win32_multi_byte_to_wide_char directly.

Fixes #966

@raphael-proust
Copy link
Collaborator

Thanks a lot for the MR. I don't have a windows machine to test this on so I can't reproduce locally. Nevertheless, I'll be reviewing this MR soon.

@mroch
Copy link
Author

mroch commented Oct 20, 2022

i dunno if you can get CI to run on the first diff (1d199cd) but it'll show the bug. for example: https://github.com/mroch/lwt/actions/runs/3284275234/jobs/5410034604

I believe the EINVAL comes from passing just a single null terminated string, without the additional \0 to terminate the block (and it's only passing the first of many envs, but I think it's the malformed block that causes the EINVAL specifically)

@raphael-proust
Copy link
Collaborator

i dunno if you can get CI to run on the first diff

I don't know either. I don't understand github's UI very well. ❓ ❗

But that's ok. When I do the review I might push a branch with just the first commit to force the CI.

Copy link
Contributor

@MisterDA MisterDA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding this and fixing the bug. The code is semantically identical to OCaml's Unix library, so I think this is good to go.
https://github.com/ocaml/ocaml/blob/c6d207656de19d97b5edf5c2f9028412aa218571/otherlibs/unix/createprocess.c#L120-L131

@raphael-proust
Copy link
Collaborator

At first I was pleased that the code was so similar.
Then I thought it'd be even better if the code was even more similar.
And then I started thinking about licenses and now I don't know what to do about this PR.

I'll ask if it's ok. Basically, the added code is calling the functions provided by the ocaml project in the appropriate sequence. I think the alternative to using the same code is making a pull-request on the ocaml project to provide this sequence of calls as a function itself to be called. That might not be desirable upstream anyway…

Anyway, I'll ask.

@raphael-proust
Copy link
Collaborator

A similar situation happened on the eio bugtracker:
ocaml-multicore/eio#352 (comment)

It's not exactly the same (not the same code being copied, not the same size of code) but it's also copying parts of the C code.

@MisterDA MisterDA mentioned this pull request Aug 4, 2023
@raphael-proust
Copy link
Collaborator

I've made raphael-proust/ocaml@bdbddd2 which aims to expose the code that we actually need from the OCaml source. Please leave comments on there, I don't know that I did anything correctly.

@raphael-proust
Copy link
Collaborator

As per ocaml/ocaml#12492 (comment) it turns out licensing is not an issue. We can go ahead on this PR.

@MisterDA
Copy link
Contributor

This is long overdue ;)

@raphael-proust
Copy link
Collaborator

Do we need to #include an additional file for windows on 5.2? The CI has the following error for that specific combination:

#=== ERROR while compiling lwt.dev ============================================#
# context     2.2.0 | win32/x86_64 | ocaml-base-compiler.5.2.0 | pinned(git+file://D:/a/lwt/lwt#HEAD#9d9efab4e28699c2f9ffd8397f19141dac199ead)
# path        D:\a\lwt\lwt\_opam\.opam-switch\build\lwt.dev
# command     D:\a\lwt\lwt\_opam\bin\dune.exe build -p lwt -j 3 @install
# exit-code   1
# env-file    D:\.opam\log\lwt-5000-9b088c.env
# output-file D:\.opam\log\lwt-5000-9b088c.out
### output ###
# File "_build/.dune/default/src/unix/dune", line 49, characters 2-19:
# 49 |   lwt_process_stubs
#        ^^^^^^^^^^^^^^^^^
# (cd _build/default/src/unix && D:\a\lwt\lwt\_opam\bin\x86_64-w64-mingw32-gcc.exe -O2 -fno-strict-aliasing -fwrapv -mms-bitfields -I. -DUNICODE -D_UNICODE -g -I D:/a/lwt/lwt/_opam/lib/ocaml -I D:/a/lwt/lwt/_opam/lib/ocaml\threads -I D:/a/lwt/lwt/_opam/lib/ocaml\unix -I D:\a\lwt\lwt\_opam\lib\bytes -I D:\a\lwt\lwt\_opam\lib\ocplib-endian -I D:\a\lwt\lwt\_opam\lib\ocplib-endian\bigstring -I ../core -o lwt_process_stubs.o -c lwt_process_stubs.c)
# In file included from D:/a/lwt/lwt/_opam/lib/ocaml/caml/memory.h:27,
#                  from lwt_process_stubs.c:21:
# D:/a/lwt/lwt/_opam/lib/ocaml/caml/domain.h: In function 'caml_check_gc_interrupt':
# D:/a/lwt/lwt/_opam/lib/ocaml/caml/domain.h:43:3: error: 'CAMLalloc_point_here' undeclared (first use in this function)
#    43 |   CAMLalloc_point_here;
#       |   ^~~~~~~~~~~~~~~~~~~~
# D:/a/lwt/lwt/_opam/lib/ocaml/caml/domain.h:43:3: note: each undeclared identifier is reported only once for each function it appears in

@dra27
Copy link
Contributor

dra27 commented Aug 7, 2024

Hmm, apparently I accidentally allowed ocaml/ocaml#11449 to be closed by the stale bot. The issue is that CAML_INTERNALS is causing more things to happen in the headers than are wanted. I wonder if this might be the best plan until an "official" API function is added: as before, only define CAML_INTERNALS for OCaml < 4.13, but instead duplicate the extern declaration from osdeps.h for caml_win32_multi_byte_to_wide_char?

It might be possible to work around it by explicitly including caml/misc.h, but I think it's better to have it that CAML_INTERNALS is only being defined for old versions of OCaml (i.e. that it's not still required with current versions)?

@raphael-proust
Copy link
Collaborator

I made a few attempts but it's a pain to work on without access to a windows machine.

I'll make a release which won't be available for 5.2 on windows (sorry) and I can make a point release later to fix this.

raphael-proust added a commit that referenced this pull request Oct 4, 2024
See #967

Version 5.8.1 will be a point fix release for availability
@raphael-proust raphael-proust mentioned this pull request Oct 28, 2024
@smorimoto smorimoto requested review from Copilot and removed request for MisterDA November 23, 2024 17:39

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (3)
  • src/unix/lwt_process_stubs.c: Language not supported
  • test/unix/dummy.ml: Language not supported
  • test/unix/test_lwt_process.ml: Language not supported
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

passing env in Lwt_process doesn't work on Windows
4 participants